-
Notifications
You must be signed in to change notification settings - Fork 87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
client: allow to authenticate with an existing refresh token #804
Conversation
🦋 Changeset detectedLatest commit: fe707b7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request has been linked to 1 task:
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@@ -35,6 +35,11 @@ export class Authentication implements IAuthentication { | |||
this.credentials = new CredentialsStorage(context.storage, context.environment.name); | |||
} | |||
|
|||
async fromRefreshToken(refreshToken: string): Promise<void> { | |||
const credentials = new Credentials(undefined, refreshToken); | |||
await this.credentials.set(credentials); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cesarenaldi I made it as simple as this.
shall we validate if the input is a valid jwt token and if it is a lens refresh token or leave it out to the consumer? if not valid then the API will fail on the refresh step anyway and it will happen the first time you call any client method. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a simple approach works for now. I have a question about the design choice.
Why you went for a method compared to a configuration option in the constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first that client.authentication
module contains all methods related to authentication, so it's easier to find it here if you are checking what is possible,
2nd, you might have a client instance from another place of your app and only authenticate later when you have a token.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's probably not clear but each form of authentication will overwrite the previous one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see and understand the rationale. The only thing is that the method name might not be intelligible. the "fromSomething" kinda make sense as static factory method (e.g. Car.fromParts(...)
) but not sure if works well as instance method of a submodule.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to better ideas 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it to
/**
* Authenticate with an existing, valid refresh token.
*/
authenticateWith({ refreshToken }: { refreshToken: string }): Promise<void>;
2e8ec77
to
eb75169
Compare
packages/client/package.json
Outdated
@@ -97,7 +97,7 @@ | |||
"typescript": "5.2.2" | |||
}, | |||
"engines": { | |||
"node": "^18.15.0" | |||
"node": ">=18" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about a range up to current stable (i.e. 20)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Hey @bigint, this change should enable you to use the |
eb75169
to
1d22c71
Compare
Amazing @cesarenaldi when this is going live? |
This week. |
1d22c71
to
cf756a1
Compare
cf756a1
to
0eb3421
Compare
ff859f6
to
fe707b7
Compare
@bigint sorry, we had some last minute things to deal with which disrupted out SDK release. Aiming to get this out early next week. |
@bigint this is now released. |
No description provided.